adapter: delete the staged cluster reconfiguration machine - #38103
adapter: delete the staged cluster reconfiguration machine#38103aljoscha wants to merge 2 commits into
Conversation
385e39c to
281b832
Compare
281b832 to
6aacdb9
Compare
| } | ||
| } | ||
| if let Some(target) = folded_target { | ||
| let Managed(target_managed) = &mut new_config.variant else { |
There was a problem hiding this comment.
should do an exhaustive destructure here please
There was a problem hiding this comment.
Done. Destructures ReconfigurationTarget so a new target dimension has to be applied here too.
| use mz_sql::plan::AlterOptionParameter::Unchanged; | ||
|
|
||
| let new_target = ReconfigurationTarget { | ||
| size: new_managed.size.clone(), |
There was a problem hiding this comment.
prolly also want exhaustive destructure here
There was a problem hiding this comment.
Done, and I did options as well as new_managed since it carries the same hazard, arguably the sharper one: a new ALTER option that names a shape dimension has to be either folded or explicitly ruled out.
The non-shape fields are spelled out with _ and a one-line reason rather than .., otherwise the destructure buys nothing. This matches alter_changes_replica_shape a few lines up, which already destructured PlanClusterOption exhaustively.
Verified it actually bites rather than just looking exhaustive: adding a probe_new_shape_dimension field to PlanClusterOption yields
error[E0027]: pattern does not mention field `probe_new_shape_dimension`
--> src/adapter/src/coord/sequencer/inner/cluster.rs:2259:9
at the new site (and at alter_changes_replica_shape). Reverted the probe, of course.
6aacdb9 to
0c59e5b
Compare
| let state = self | ||
| .observe_cluster_state(cluster_id) | ||
| .expect("managed cluster observed above"); | ||
| reconcile_replicas(&state, &[desired]) |
There was a problem hiding this comment.
One thing I am not quite understanding here...
My understanding was that the controller passes one contribution per strategy into as the desired state (including HydrationBurst). Since a burst replica is owned and has a different size, will reconiciliation drop the burst replica if the wait for is 0s?
If so, would that leave the burst record in place?
Was there silent guarding of this behaviour on the old code path before, with the full reconciliation?
I see some test changes as well in similar scenarios, but not quite sure ...
Either way flagging it.
There was a problem hiding this comment.
You found a bug! This was somewhat working before, and even is now, but it's tricky:
- Before, even a wait = 0s, commit cutover would go through the controller, and it would do the correct thing.
- I decided to keep this non-controller path for wait = 0s, commit, as a failsafe, but it's complicating things slightly because it duplicates the decision logic, and it did it incorrectly 🙈
What would happen right now: we drop the burst replica, we keep the burst record. On the next controller tick the burst replica is re-created, but we have lost it's state, of course.
I'm slightly leaning towards keeping this reconfiguration path, and making sure it works correctly by re-using the same decision code. But I could probably be convinced to remove it, and let the controller handle the wait =0s, commit path as well. Wdyt?
There was a problem hiding this comment.
:) Agreed on keeping it! I think if we can somehow refactor to maximize code reuse across the code path that is good as well.
With the controller owning every managed cluster's replica set, `NeedsFinalization::Yes` has no producer. Delete the machine it drove: the `WaitForHydrated` and `Finalize` stages, the `-pending` overlap replicas, the `pending_cluster_alters` connection state and its retire paths, and the `AlterClusterWhilePendingReplicas` error. The direct reshape path is deliberately kept, as the synchronous cut-over. It is now routed to by an explicitly zero-timeout commit strategy (`WITH (WAIT FOR '0s')`, or `WAIT UNTIL READY (TIMEOUT '0s', ON TIMEOUT 'COMMIT')`) rather than by the absence of a `WAIT` clause. Two reasons. It is the escape hatch: every other reshape depends on the controller ticking and applying, and this is the one that still works when the controller itself is the problem, while also unsticking a wedged reconfiguration by retiring its record. And the semantics are honest: a zero timeout with commit already means "cut over now, hydrated or not", so doing it synchronously in the ALTER is the same outcome minus a tick. "The same outcome" has to be true, so the cut-over does not improvise. It folds its target onto an in-flight one exactly as the reshape path does, and it converges the replica set with the controller's own reconcile kernel, so a replica that already has the target shape is kept rather than bounced. Forcing a stuck-but-hydrating resize to commit therefore keeps the replica that was already up, and lands the record on `finalized` (forced) rather than `cancelled`, since the cut-over reached the record's own target. Creating a replica from the controller's `ReplicaShape` also drops a lossy round-trip through the planner's `ComputeReplicaConfig`, which cannot represent `INTROSPECTION DEBUGGING` without an interval. A cluster in that state used to get a replica whose logging disagreed with the config that called for it.
Follow-ups on deleting the staged reconfiguration machine, no behavior change. Collapse the cut-over routing's bool-plus-Option pair into a single optional cut-over target, computed unconditionally on that path. The fold with nothing in flight returns exactly the statement's shape, so applying it is the identity and the "nothing in flight, nothing to fold" special case disappears. The cut-over now computes its target through the same call the reshape path makes, so the two agreeing is structural rather than commented. Applying a target onto a managed config is the inverse of `realized_reconfiguration_target`, so it moves next to it in objects.rs as `apply_reconfiguration_target`, keeping the exhaustive-destructure compile guard beside the type it guards. `sequence_create_managed_cluster` builds the one `ReplicaShape` all its replicas share once, instead of a separate logging binding plus a per-iteration shape construction in the loop. Also fix the `sequence_alter_cluster_managed_to_managed` doc, which claimed the carried record is retired as cancelled and the replica set dropped wholesale: `retire_carried_reconfiguration` settles an on-target record as finalized (forced), and the reconcile keeps already-matching replicas.
e115a22 to
86f1766
Compare
|
Heads up: #38139 (just merged) adds a "single-replica sources on a multi-replica cluster" notice with emission points inside
One non-conflict to also pick up: the doc comment on |
| /// | ||
| /// A zero timeout that rolls back is *not* this: it asks for the reconfiguration | ||
| /// to be abandoned at once, which is the record path's job. | ||
| fn requests_immediate_cut_over(strategy: &AlterClusterPlanStrategy) -> bool { |
There was a problem hiding this comment.
The AlterClusterReplicationFactorWhileReconfiguring refusal still fires for the immediate cut-over, so the escape hatch can't change the factor while unsticking a wedged record (the new test's factor case has no record in flight). The refusal's rationale, silent clobber at the async cut-over, doesn't apply here: the cut-over retires the record in the same transaction, and nothing consumes a retired record's target. Consider exempting requests_immediate_cut_over, or noting the two-step workaround (cut over the shape first, then alter the factor) in the refusal's comment.
| let mut final_config = ClusterConfig { | ||
| variant: ClusterVariant::Managed(new_config), | ||
| workload_class: workload_class.clone(), | ||
| }; |
There was a problem hiding this comment.
nit: this was the last producer of Op::UpdateClusterReplicaConfig, so the op variant, its transact.rs apply arm, and the ddl.rs audit match arm can go with the machine too (here or in a follow-up).
3 participants
Part 3 of 3 of the design for removing the legacy cluster paths.
Stacked on #38102.
Why
With the controller owning the replica set of every managed cluster (parts 1
and 2),
NeedsFinalization::Yeshas no producer left, so the stagedreconfiguration machine it drove is unreachable.
What is deleted
ClusterStage::WaitForHydratedandClusterStage::Finalize, their structs,dispatch arms, and handlers (
check_if_pending_replicas_hydrated_stage,finalize_alter_cluster_stage).NeedsFinalizationandPENDING_REPLICA_SUFFIX, and thependingparameterof
create_managed_cluster_replica_op.pending_cluster_altersconnection state and its retire paths:drop_reconfiguration_replicas,retire_cluster_reconfigurations_for_conn,cancel_cluster_reconfigurations_for_conn, and their call sites inconnection cleanup and cancellation.
AlterClusterWhilePendingReplicaserror (its raise site goes with themachine, its catch site went with the scheduler in part 1).
sequence_alter_cluster_managed_to_managed: the pending-replica arm andthe separate scale-up/down branches. A factor-only change is now a
config-only write the controller converges, like a user cluster's today.
What is kept, and why
The whole-set recreate branch stays, as the direct cut-over path: retire any
carried reconfiguration record as cancelled, write the target config, and
synchronously drop the observed owned replica set and recreate it at the target
shape and factor, all in one catalog transaction with no controller
involvement.
It is now routed to by an explicitly zero-timeout commit strategy
(
WITH (WAIT FOR '0s'), orWAIT UNTIL READY (TIMEOUT '0s', ON TIMEOUT 'COMMIT')) instead of byAlterClusterPlanStrategy::None. Two reasons:every other reshape depends on the controller ticking and applying. The
direct path is the one reshape that still works when the controller itself is
the problem, and it simultaneously unsticks a wedged reconfiguration by
retiring the record. Requesting it under a live controller stays safe: the
config write invalidates any in-flight tick's compare-and-append witness, so a
stale controller batch is rejected, the same as any user DDL landing mid-tick.
hydrated or not". Doing it synchronously in the ALTER instead of one
controller tick later is the same outcome, minus the tick.
A zero timeout that rolls back is not this: it asks for the reconfiguration
to be abandoned at once, which is the record path's job, so it keeps writing a
record.
AlterClusterPlanStrategyandClusterStage::AwaitReconfigurationstay (thecontroller reshape path and its foreground wait-shim consume them, as does
cluster_alter_check_ready_interval), as doremove_pending_cluster_replicas_migration(now the only remaining cleaner forreplicas stranded durably
pendingby a crash on a pre-deletion version, so itretires later together with the durable field) and
reconcile_builtin_cluster_replicas.Behavior change
WITH (WAIT FOR '0s')used to write a record the controller committed on itsnext tick. It now returns with the cut-over already transacted. Same outcome,
one tick sooner, and it works even when the controller does not.
Tests
test/testdrive/cluster-controller.td: a new "Direct cut-over: thezero-timeout commit escape hatch" section. A sleeping materialized view pins
hydration so the controller can never cut over, leaving a wedged
reconfiguration with a stranded overlap replica;
WITH (WAIT FOR '0s')thenreturns with the realized config advanced, exactly one replica at the target
size, the record
cancelled, and astarted->cancelledaudit trail. Asecond case covers the
WAIT UNTIL READY (TIMEOUT '0s', ON TIMEOUT 'COMMIT')spelling with a factor change riding along. This replaces the
cc_strandbreak-glass section part 1 removed.
cc_force_existing,cc_rollback) move fromTIMEOUT '0s'toTIMEOUT '1ms', so they keepexercising the controller's past-deadline commit rather than being rerouted
to the direct path.
test/sqllogictest/managed_cluster.slt: the zero-timeoutWAITcases nowread back the size (and, where a factor rides along, the replica count)
without retrying, which is exactly what the synchronous path buys.